Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions (waiting on @annrpom)

record/log_writer_latency_test.go line 17 at r2 (raw file):

)

func TestWALMetricsWithErrorfsLatency(t *testing.T) {

Is this test reliable? In general, relying on timings leads to flakes. Can you run it with --exec 'stress -p 64' and with --exec 'stress -p 64' --race?

More generally, this is a LOT of testing code. ~1500 lines of code for a relatively simple addition - these tests will need to be maintained, sometimes debugged. I don't think it's worth it. Some things are hard to add unit tests for and that's ok, especially when it's an observability thing that has no bearing on correctness.

record/log_writer.go line 837 at r2 (raw file):

		writeStart := crtime.NowMono()
		_, err = w.w.Write(data)
		if writeLatency := writeStart.Elapsed(); w.flusher.walFileMetrics.WriteLatency != nil {

[nit] the assignment here is strange (it's not used in the condition). I'd just inline writeStart.Elapsed().

I haven't reviewed the test code yet, but in a cursory glance I found the sheer volume a little daunting. We try to hold our test code to a similar standard of simplicity as production code, often uncoupling test data/cases from test code that performs the mechanics. I'm wondering if we can test these changes with less code, perhaps leaning on either datadriven tests (uncoupling data and test code) or randomized testing (which tends to achieve higher coverage with less test code to maintain).

Reviewable status: 0 of 19 files reviewed, 3 unresolved discussions (waiting on @annrpom and @xxmplus)

metrics.go line 568 at r2 (raw file):

		CloseLatency   prometheus.Histogram // File close operations
		StatLatency    prometheus.Histogram // File stat operations
		OpenDirLatency prometheus.Histogram // Directory open operations

I don't think we need per-operation histograms, but rather one histogram for the primary and one histogram for the secondary, reporting every operation against the individual file.

will do.

Reviewable status: 0 of 19 files reviewed, 3 unresolved discussions (waiting on @annrpom, @jbowens, and @RaduBerinde)

metrics.go line 568 at r2 (raw file):

These ops have quite different latency and frequency access patterns. If we consolidate them into one, we will lose visibility to the less frequent ops. Is that still ok?

record/log_writer_latency_test.go line 17 at r2 (raw file):

I ran it against stress but not race. And ok, I will remove some verbose tests.

@jbowens reviewed 1 of 2 files at r1, 1 of 17 files at r4, all commit messages.
Reviewable status: 2 of 19 files reviewed, 7 unresolved discussions (waiting on @annrpom, @RaduBerinde, and @xxmplus)

open.go line 295 at r4 (raw file):

	walFileOpHistogram := prometheus.NewHistogram(prometheus.HistogramOpts{
		Name:    "pebble_wal_file_op_duration_nanoseconds",
		Help:    "Histogram of WAL file operation latencies in nanoseconds",

nit: I think we should just exclude the Name and Help and sections; it'll be unused in the context of Cockroach and may confuse

metrics.go line 578 at r4 (raw file):

		prometheus.LinearBuckets(0.0, 0.01, 50),                 // 0 to 0.5ms in 10μs increments
		prometheus.ExponentialBucketsRange(1.0, 10000.0, 50)..., // 1ms to 10s
	)

I think we can unify the histogram configuration, consolidating FsyncLatencyBuckets and WALFileOpLatencyBuckets

db.go line 383 at r4 (raw file):

				// protected by mu.
				fsyncLatency prometheus.Histogram
				// Updated whenever a wal.Writer is closed.

nit: can we keep the comment that metrics.LogWriterMetrics is updated when a WAL writer is closed?

wal/failover_writer.go line 463 at r4 (raw file):

	// WAL file operation latency histogram
	walFileOpHistogram record.WALFileOpHistogram
	directoryType      record.DirectoryType

the failover handles writing to both the primary and secondary. I think we need access to both histograms and be able to construct child LogWriters that use one or the other depending on which device the child LogWriter is writing to.

Reviewable status: 2 of 19 files reviewed, 7 unresolved discussions (waiting on @annrpom, @jbowens, and @RaduBerinde)

db.go line 383 at r4 (raw file):

ack. added to WALMetrics definition in metrics.go

metrics.go line 578 at r4 (raw file):

ack. done.

open.go line 295 at r4 (raw file):

removed.

wal/failover_writer.go line 463 at r4 (raw file):

ack.